Feature: Use Mantine ScrollArea #485
Conversation
…her/mantine-react-table into feature/mantine-scrollArea
|
@studylog-sbasher is attempting to deploy a commit to the Kevin Vandy OSS Team on Vercel. A member of the Team first needs to authorize it. |
packages/mantine-react-table/src/components/table/MRT_TableContainer.tsx
Outdated
Show resolved
Hide resolved
packages/mantine-react-table/src/hooks/useMRT_ColumnVirtualizer.ts
Outdated
Show resolved
Hide resolved
packages/mantine-react-table/src/hooks/useMRT_RowVirtualizer.ts
Outdated
Show resolved
Hide resolved
| } | ||
| : {}; | ||
|
|
||
| const ScrollWrapper = withScrollArea ? Table.ScrollContainer : Fragment; |
There was a problem hiding this comment.
Should the ScrollWrapper just go in the TableContainer component and replace the default Box if enabled?
There was a problem hiding this comment.
I believe that there were CSS issues / interactions in certain edge cases that were introduced by swapping it out 1:1 which would need to be mitigated - I will do additional investigation to see what effort would be needed to see if we can swap this Box component for the scroll container as that would be simpler for maintainability and would make more sense IMO
There was a problem hiding this comment.
Verified - It does behave weird without a larger refactor, the current implementation appears to be the least invasive approach with the smallest amount of side effects. Happy to try to spike on this further but I'm not sure it's worthwhile.
| const scrollContainerProps = withScrollArea | ||
| ? { | ||
| className: classes.scrollContainer, | ||
| h: '100%', |
There was a problem hiding this comment.
There's a bug where the scroll area will not grow in size when the table height grows, such as when a table enters full-screen
There was a problem hiding this comment.
I think this was just a side-effect of setting max-height: 500px on the stories - removing that, it seems to behave correctly (though I'll just remove the max height since it's a different behavior than the rest of the story examples to avoid confusion)
|
Also, since we're still in beta, we could just totally replace the current scrolling setup and just go with the stock mantine one. It would be a minor (mostly non-breaking) change before a stable release. It's a bit complicated with the two different ways. |
|
Hey @sbasher314 just checking if you're still available to continue working in this PR? |
Hey yeah, started a new job so things have been pretty busy but I should have capacity to work on this shortly. I should be able to make the requested changes |
|
I second using Mantine's ScrollArea and dropping native scrolling entirely — smart decision. We're all invested in Mantine anyways so it makes MRT just that much more cohesive. |
Amazing feature proposal, thank you! |
|
Alright sorry for the delay folks! I've been putting off getting this setup as it was previously on my old work laptop and was caught up with the new job. I've pulled this project down and will start making the requested changes - I've already brought my fork / branch up to date with the current v2 branch and will ensure that all functionality / asks have been resolved. It looks like some of the unrelated bugs I addressed have already been addressed in different ways upstream (such as the |
|
Deployment failed with the following error: |
|
@KevinVandy I believe that I should have addressed all comments / feedback as originally put forward -- there's still an open discussion here whether to use the scroll container by default or to make it opt-in. I think that there's a strong argument for performance and accessibility purposes that we would want to default to the current behavior, but I'm open to alternative thoughts on that matter. It does complicate the code by having this be conditional like this. A middleground approach is to have the |
|
@KevinVandy 🧑🍳 👀 |
6 participants
Adds Mantine
ScrollAreacomponent as an optional wrapper for the table body; Supports virtualization as well as standard / non-virtualized tables.Usage:
withScrollAreato the table props to enable this feature -- disabled by default so as to not break back-compatibilitymantineScrollAreaPropsfor further customization if neededI've fleshed out a few storybook examples as well as a few examples on the docs (specifically virtualization / infinite loading). Let me know if I missed anything or if there's any room for improvement!
Address the following discussion / issue : #3